Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add connecting to database by connectionString given as an argument to cli #120

Merged
merged 13 commits into from
Dec 7, 2024

Conversation

Kuchteq
Copy link

@Kuchteq Kuchteq commented Nov 12, 2024

Thanks for the amazing tool!
I'd like to see a feature like this get merged as I find it easier to just pass the path to the database I want to open as an argument, so I made this request. This implementation works but perhaps is doing a bit too much in the main.go file. I first tried doing most of this connection initialization inside Pages.go but the init() functions run before main.go's main() function.

I'd love to hear your feedback on this!

main.go Outdated Show resolved Hide resolved
@jorgerojas26
Copy link
Owner

I think your intuition is right, the main.go file need to be keep small, can you please move the command args processing to another file?. Also, can you please expand the "usage" section in the readme to include this?

Thank you for the PR.

components/ArgConnection.go Outdated Show resolved Hide resolved
components/ArgConnection.go Outdated Show resolved Hide resolved
@Kuchteq
Copy link
Author

Kuchteq commented Nov 19, 2024

I am facing an issue where only sqlite databases work with passing the argument. Postgresql connects without any problem but can't open unfold the left menu for listing the tables.

@jorgerojas26
Copy link
Owner

jorgerojas26 commented Nov 30, 2024

I have tested MySQL, Postgres and SQLite and it works fine. Is there anything else you want to add or are we ready for merge?

@Kuchteq
Copy link
Author

Kuchteq commented Dec 1, 2024

I don't have anything more. I'd be happy to see it merged :)

components/ArgConnection.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@jorgerojas26
Copy link
Owner

Please fix linter errors.

main.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@jorgerojas26
Copy link
Owner

Thank you @ccoVeille for the good advices. I'm learning too, so thanks for taking the time to keep an eye on the project.

@jorgerojas26 jorgerojas26 merged commit d4cd01f into jorgerojas26:main Dec 7, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants